-
Notifications
You must be signed in to change notification settings - Fork 480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add multi-instance option support globally and per option #625
base: develop
Are you sure you want to change the base?
Add multi-instance option support globally and per option #625
Conversation
Renamed the global AllowMultiInstance setting to AllowMultiInstanceByDefault as this value is used to define the default action to take when the per-option AllowMultiInstance isn't specified Added extra test to test the AllowMultiInstance option on options Added extra test to make sure parsing fails on normal non(null)-AllowMultiInstance options with the global setting set to false as expected
While I'm not the maintainer, nullable boolean arguments don't fit with the code style. If you really have to have a three-valued bool (true, false, or not specified) for allowMultiInstance in OptionSpecification, I'd suggest using a |
Doing the not specified = false would make it impossible to enable multi instance by default and opt-out per option though. Also, if I read Maybe correctly, if I'd use Maybe.Return(false) it would actually return an instance of Nothing |
Ah, right. Then a |
Okay, looks like there's a bug in Maybe. The |
I'll see if I can make the requested changes. I'm not the biggest fan of using Maybe for this, but I'll see if I can comply. |
@rmunn Hmm, Maybe isn't public so I cannot use this type for the public AllowMultiInstance read-only option on OptionAttribute, should I make that property internal? edit: Maybe I should retain the nullable bool there and change everywhere else to use this 'Maybe'? Like in FromAttribute and OptionSpecification |
…tor instead of just once
If it's a property on OptionAttribute, then it has to be public, since that's what library users will be accessing when they write |
AllowMultiInstance cannot be a nullable bool then though (not allowed in attributes). Making it only set the nullable bool internally on set could work (be it false or true) but only for writing. For calculating the effective value, you will need the current parser' context to get that setting, not sure how you would feed that info to the option? |
This PR continues on the work done in PR #594 and makes the following changes:
Any feedback is welcome.